Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix-component-undefined-svelte-v5 #12102

Merged
merged 4 commits into from
Oct 5, 2024
Merged

Fix-component-undefined-svelte-v5 #12102

merged 4 commits into from
Oct 5, 2024

Conversation

hermit99
Copy link
Contributor

@hermit99 hermit99 commented Oct 2, 2024 •
edited
Loading

Changes

Small code enhancement, which is easy to understand.

  • Fix 'component is not defined' console error when unmounting svelte 5 component.

Console error: Screenshot 2024-10-01 110532

Testing

Defensive code added and tested locally.

Docs

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Oct 2, 2024 •
edited
Loading

🦋 Changeset detected

Latest commit: ffa87ee

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added pkg: svelte Related to Svelte (scope) pkg: integration Related to any renderer integration (scope) labels Oct 2, 2024
@bluwy bluwy mentioned this pull request Oct 4, 2024
Merged
@martrapp
Copy link
Member

martrapp commented Oct 5, 2024

Thanks @bluwy for pointing out!

@hermit99, thank you very much for providing this PR!
I'll have a look at #12116 and we might fix them both in the same way as the issues result from the same refactoring.

martrapp
martrapp requested changes Oct 5, 2024
View reviewed changes
Copy link
Member

@martrapp martrapp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @hermit99, the suggestion would be to revert the file and only move the addEventlistener from line 44 up to line 42 at the end of the else-block. That would ensure that component is always defined when referenced and would also fix the issue that the listener is currently added too often

hermit99 and others added 2 commits October 5, 2024 08:34
@hermit99
Moving unmount listener to where the component is defined.
88d8d88
@hermit99 @martrapp
Update .changeset/eighty-ligers-punch.md
ffa87ee 
Co-authored-by: Martin Trapp <94928215+martrapp@users.noreply.github.com>
@hermit99
Copy link
Contributor Author

hermit99 commented Oct 5, 2024

Thanks @martrapp. Pardon me as I'm new to Astro's code base. Your suggestion makes more sense, and I really appreciate it ❤️

@martrapp
Copy link
Member

martrapp commented Oct 5, 2024

Thank you very much for taking care! 💜
The suggestion came from #12116 (comment)

@martrapp martrapp merged commit dcc1e89 into withastro:next Oct 5, 2024
12 of 13 checks passed
@astrobot-houston astrobot-houston mentioned this pull request Oct 5, 2024
Merged
@hermit99 hermit99 deleted the fix-component-undefined-svelte-v5 branch October 5, 2024 10:04
martrapp added a commit that referenced this pull request Oct 5, 2024
@hermit99 @martrapp
Fix-component-undefined-svelte-v5 (#12102)
6c02d15 
* - fix: 'component is not defined' error when unmount svelte 5 component

* added changeset

* Moving unmount listener to where the component is defined.

* Update .changeset/eighty-ligers-punch.md

Co-authored-by: Martin Trapp <94928215+martrapp@users.noreply.github.com>

---------

Co-authored-by: Martin Trapp <94928215+martrapp@users.noreply.github.com>
martrapp added a commit that referenced this pull request Oct 5, 2024
@martrapp @hermit99
Fix-component-undefined-svelte-v5 (#12102) (#12129)
5bc6223 
* - fix: 'component is not defined' error when unmount svelte 5 component

* added changeset

* Moving unmount listener to where the component is defined.

* Update .changeset/eighty-ligers-punch.md



---------

Co-authored-by: Hermit <70563349+hermit99@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martrapp martrapp martrapp approved these changes

Assignees
No one assigned
Labels
pkg: integration Related to any renderer integration (scope) pkg: svelte Related to Svelte (scope)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hermit99 @martrapp